refactor: replace verify_interface with isinstance #467
Conversation
There was a problem hiding this comment.
Looks good. Left a nit.
I am going to dig into what verify_interface actually does...
Is there a reason we originally used verify_interface instead of isinstance?
Did @josecorella look into this?
If so, can you update the PR's description to mention your findings?
UPDATE: I had not realized that pyca had already suggested to us that isinstance offers the same guarantees, at least in our use cases.
| def test_signer_from_key_bytes(patch_default_backend, patch_serialization, patch_build_hasher, patch_ec): | ||
| mock_algorithm_info = MagicMock(return_value=sentinel.algorithm_info, spec=patch_ec.EllipticCurve) | ||
| _algorithm = MagicMock(signing_algorithm_info=mock_algorithm_info) | ||
| # _algorithm = MagicMock() |
There was a problem hiding this comment.
nit: remove left over commented out code
|
Hi! Due to this refactor an error NotSupportedError("Unsupported signing algorithm info") is raised when decrypting with some Algorithms with version of cryptography=2.8. I upgrade the cryptography version to the latest (41.0.7 right now) and the error was gone. I dont know from what version the issue is resolved. I suggest to increase the required version of cryptography in your guide. Algorithm with issue: AlgorithmSuite.AES_256_GCM_HKDF_SHA512_COMMIT_KEY_ECDSA_P384 Error log: |
Issue #, if available:
#464
Description of changes:
pyca/cryptographyis planning to removeverify_interface. However; since they run the esdk as downstream in ci, they can't deprecate the function without breaking their CI. This PR is intended to unblock them by removingverify_interfacefrom the ESDK.verify_interfacetakes an abstract class in this caseec.EllipticCurveand the subclass object you wish to verify, in our use case it'salgorithm.signing_algorithm_infowhich is a subclass that implements theec.EllipticCurveabstract class. The only allowed subclasses we use areec.SECP256R1andec.SECP384R1andverify_interfacegoes through the class and makes sure that the expected values are there. Since both of the allowed subclasses implementec.EllipticCurve, usingisinstanceworks as a replacement.I noticed that the test_vector_handlers static analysis action is failing because of linter dependency issues. I went ahead and updated it to use the pinned dependencies in
dev_requirements/. If these changes don't make sense to include in the same PR I am happy to break them up into two :)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: